-
-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Select] Create new Select
component
#541
base: master
Are you sure you want to change the base?
Conversation
Netlify deploy preview |
2384edd
to
c32adfc
Compare
ownerState, | ||
propGetter: (externalProps) => getTriggerProps(getRootTriggerProps(externalProps)), | ||
customStyleHookMapping: commonStyleHooks, | ||
extraProps: otherProps, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if the Trigger had a CSS variable with the width of the popup, so it can match it (as in native OS controls)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the inverse? --anchor-width
for popup is available. The trigger can't know the width of the popup when it's not mounted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I didn't mean the inverse. Native OS selects adapt their width to the longest option. Perhaps this could be an option when keepMounted=true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, but I'm hesitant to add something that doesn't work by default. Furthermore, it's likely very performance intensive with longer lists to calculate as you'll need to measure every option with getBoundingClientRect
to calculate the largest one. It also doesn't work during SSR, so will cause layout shift. It's likely something that should be hardcoded by the user if necessary.
Could you please review the list of reported Select bugs and set this PR to close the ones that will be fixed by it? |
When you open the select, the page scrollbar disappears causing a layout shift on the right navbar in the doc (https://deploy-preview-541--base-ui.netlify.app/components/react-select/) The |
@flaviendelangle interesting. The |
a0c48cc
to
a4334d8
Compare
I'm wondering if we should prevent scrolling by default at all. I find it annoying when I open a select and something else changes appearance (=the scrollbar disappears). Native implementations differ in this matter - iOS prevents scrolling but doesn't hide the scrollbar, while Windows closes the popup when the page scrolls. I found an issue that's likely related to scroll locking: https://codesandbox.io/p/sandbox/q7qfjg - when you open the select, there's a small layout shift and you're able to scroll the page horizontally until the whole trigger disappears. |
The scroll locking changes we made in #604 prevents layout shift issues from disappearing scrollbars. The scroll lock is necessary with the item align anchoring because you can
This looks like a bug with the .SelectPopup This solves it: min-width: min(var(--available-width), calc(var(--anchor-width) + 20px)); There's a thread discussing default "functional styles" to apply to these elements and an API to override them in a thread in the base-ui channel. This could help prevent certain issues like this, including:
|
The repro was taken directly from the demos, so they'll need to have the styles updated. |
28ffec5
to
a8bed12
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous implementation supported multiple selection. Do you plan to add it here as well or in a separate component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the potential complexity, a separate component + we'd need to discuss such an API first
packages/mui-base/src/Tooltip/Positioner/useTooltipPositioner.ts
Outdated
Show resolved
Hide resolved
This is the most common default, though Radix uses the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good (assuming the tests are fixed)!
Preview: https://deploy-preview-541--base-ui.netlify.app/components/react-select/
Select.Value
placeholder
prop)defaultValue
/value
don't match any of the options)TODO:
useField
alignMethod
prop toalignOptionToTrigger
Olivier's edit: the v2 of mui/material-ui#8023 😄